Conversation
- Refactored IControllerEnvironmentContext.GetEnvironment to support a prefix option and updated all usages. - ControllerEnvironmentContext now always stores all variables in the command-specific environment and supports prefixing on retrieval. - CommandController updated to distinguish between global and command-specific environment updates. - Added CommandRegistryGetEnvironmentTests for comprehensive coverage of environment collection logic. - Introduced TestCommandsWithEnvironment.cs with diverse ICommandDelegate test implementations. - Updated all affected unit tests to match new environment logic and method signatures. - Enhanced EchoCommandDelegate and TestCommandDelegate to support default environment variables.
There was a problem hiding this comment.
Pull request overview
Adds a way to build a composite controller environment by instantiating registered commands and collecting their GetDefaultEnvironment() values, alongside updates to environment prefixing/merging behavior and associated tests.
Changes:
- Added
ICommandRegistry.GetEnvironment(ICommandFactory)+CommandController.GetEnvironment()to collect default env from registered commands. - Updated
IControllerEnvironmentContext.GetEnvironment(...)to support optional key prefixing and adjusted controller/environment merge logic accordingly. - Added/updated tests and test command implementations to exercise environment collection and prefixing.
Reviewed changes
Copilot reviewed 19 out of 21 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/Xcaciv.Command.Tests/TestImplementations/TestCommandsWithEnvironment.cs | Adds many ICommandDelegate test commands with representative default environment dictionaries. |
| src/tests/Xcaciv.Command.Tests/TestImpementations/TestCommandDelegate.cs | Extends test delegates to support injectable default environments. |
| src/tests/Xcaciv.Command.Tests/ControllerEnvironmentContextTests.cs | Updates expectations/calls for new GetEnvironment(commandName, prefix) behavior and revised update semantics. |
| src/tests/Xcaciv.Command.Tests/Commands/RegifCommandTests.cs | Fixes indentation/formatting in a test method signature. |
| src/tests/Xcaciv.Command.Tests/CommandRegistryGetEnvironmentTests.cs | New tests validating registry-driven default environment collection. |
| src/tests/Xcaciv.Command.FileLoader.Tests/EnvironmentFileManagerTests.cs | Updates mock IControllerEnvironmentContext signature to match interface change. |
| src/Xcaciv.Command/Xcaciv.Command.csproj | Version bump to 3.3.3. |
| src/Xcaciv.Command/PipelineExecutor.cs | Removes an extra blank line (formatting). |
| src/Xcaciv.Command/ControllerEnvironmentContext.cs | Adds prefix option to command env retrieval and changes how per-command updates are stored. |
| src/Xcaciv.Command/CommandRegistry.cs | Implements environment collection by creating commands and aggregating defaults. |
| src/Xcaciv.Command/CommandController.cs | Adjusts parent environment reintegration rules and adds GetEnvironment() API. |
| src/Xcaciv.Command.Interface/Xcaciv.Command.Interface.csproj | Version bump to 3.3.3. |
| src/Xcaciv.Command.Interface/IControllerEnvironmentContext.cs | Changes GetEnvironment(commandName) signature to include optional prefix. |
| src/Xcaciv.Command.Interface/ICommandRegistry.cs | Adds GetEnvironment(ICommandFactory) to the public interface. |
| src/Xcaciv.Command.Interface/ICommandController.cs | Adds GetEnvironment() to the public interface. |
| src/Xcaciv.Command.FileLoader/Xcaciv.Command.FileLoader.csproj | Version bump to 3.3.3. |
| src/Xcaciv.Command.DependencyInjection/Xcaciv.Command.DependencyInjection.csproj | Version bump to 3.3.3. |
| src/Xcaciv.Command.Core/Xcaciv.Command.Core.csproj | Version bump to 3.3.3. |
| src/Directory.Packages.props | Updates referenced package versions to 3.3.3. |
| private static void ApplyDefaultEnvironment(IControllerEnvironmentContext controllerEnvironment, ICommandDescription commandDescription, ICommandFactory commandFactory, int depth = 0) | ||
| { | ||
| depth++; | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(commandDescription.FullTypeName)) | ||
| { | ||
| AddCommandDefaults(controllerEnvironment, commandDescription, commandFactory); | ||
| } | ||
|
|
||
| if (commandDescription.SubCommands.Count == 0) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| if (depth > 1) return; | ||
| foreach (var subCommand in commandDescription.SubCommands.Values) | ||
| { | ||
| AddCommandDefaults(controllerEnvironment, subCommand, commandFactory); | ||
| } | ||
| } |
There was a problem hiding this comment.
ApplyDefaultEnvironment increments a depth parameter and then immediately blocks traversal when depth > 1, but the method never recurses (it only iterates direct SubCommands). This makes the depth logic misleading/dead code; either remove it or implement true recursive traversal if deeper nesting is expected.
| if (!prefix) return commandEnv.ToDictionary(); | ||
| // Add command prefix to keys when retrieving | ||
| var commandPrefix = string.Concat(commandName, "_"); | ||
| var result = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | ||
| foreach (var (key, value) in commandEnv) | ||
| { | ||
| var prefixedKey = key.StartsWith(commandPrefix, StringComparison.OrdinalIgnoreCase) ? key : commandPrefix + key; | ||
| result[prefixedKey] = value; | ||
| } | ||
| return result; | ||
| } | ||
| return new Dictionary<string, string>(); |
There was a problem hiding this comment.
GetEnvironment(commandName, prefix: false) returns commandEnv.ToDictionary(), which creates a case-sensitive dictionary. This breaks the environment context’s case-insensitive contract and can cause lookups to behave differently depending on the prefix flag. Return a Dictionary<string,string> using StringComparer.OrdinalIgnoreCase (and do the same for the empty/not-found return path).
| if (!prefix) return commandEnv.ToDictionary(); | |
| // Add command prefix to keys when retrieving | |
| var commandPrefix = string.Concat(commandName, "_"); | |
| var result = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | |
| foreach (var (key, value) in commandEnv) | |
| { | |
| var prefixedKey = key.StartsWith(commandPrefix, StringComparison.OrdinalIgnoreCase) ? key : commandPrefix + key; | |
| result[prefixedKey] = value; | |
| } | |
| return result; | |
| } | |
| return new Dictionary<string, string>(); | |
| if (!prefix) | |
| { | |
| var result = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | |
| foreach (var environmentEntry in commandEnv) | |
| { | |
| result[environmentEntry.Key] = environmentEntry.Value; | |
| } | |
| return result; | |
| } | |
| // Add command prefix to keys when retrieving | |
| var commandPrefix = string.Concat(commandName, "_"); | |
| var resultWithPrefix = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); | |
| foreach (var (key, value) in commandEnv) | |
| { | |
| var prefixedKey = key.StartsWith(commandPrefix, StringComparison.OrdinalIgnoreCase) ? key : commandPrefix + key; | |
| resultWithPrefix[prefixedKey] = value; | |
| } | |
| return resultWithPrefix; | |
| } | |
| return new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase); |
| @@ -232,39 +243,20 @@ public void UpdateEnvironment(Dictionary<string, string> dictionary, string comm | |||
| return; | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
The XML remarks for UpdateEnvironment(Dictionary<string,string>, string commandName) still describe splitting updates into command-specific (prefixed) vs shared/global, but the implementation now stores all keys in the command-specific dictionary and never updates _environment. Please update the documentation to match the new behavior, or restore the prior behavior if shared/global updates are still intended.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Improved environment update logic to distinguish between global and command-specific variables. Now, command-prefixed variables are filtered out before global updates, and command-specific updates strip the prefix before storage. This prevents accidental overwrites and ensures clear separation between global and command environments.
Revised unit tests for ControllerEnvironmentContext to match updated environment variable key conventions: command-specific keys now omit redundant prefixes, while shared keys are prefixed with the command name. Adjusted test assertions and descriptions accordingly.
No description provided.